Skip to content

Use a fake GADT for sequence folds and traversals #1078

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

treeowl
Copy link
Contributor

@treeowl treeowl commented Dec 15, 2024

No description provided.

@treeowl
Copy link
Contributor Author

treeowl commented Dec 15, 2024

As promised, here's the working draft. It's incomplete and completely unbenchmarked.

@@ -1078,7 +1135,33 @@ instance Sized a => Sized (FingerTree a) where
size (Single x) = size x
size (Deep v _ _ _) = v

-- We don't fold FingerTrees directly, but instead coerce them to
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lie at the moment. It might or might not be true later. WIP....

@treeowl
Copy link
Contributor Author

treeowl commented Dec 15, 2024

@augustss I'm sure this code is nonsense under MicroHs, but the error message it gives me looks equally nonsensical.

@augustss
Copy link
Contributor

augustss commented Dec 15, 2024 via email

@meooow25
Copy link
Contributor

It's not just MicroHs, I've never seen GHC react in this many ways to the same piece of code 😀

  • 8.2, 8.8 - panic! (the 'impossible' happened)
  • 9.0 - Error: [Cabal-7125]
  • 8.4, 8.6, 8.10 - Test failure
  • >= 9.2 - OK

@treeowl
Copy link
Contributor Author

treeowl commented Jul 11, 2025

This still isn't benchmarked, but it's much closer to complete. The main thing I haven't tried applying it to is liftA2.

@treeowl
Copy link
Contributor Author

treeowl commented Jul 11, 2025

@meooow25 Do you have any general thoughts?

@treeowl treeowl force-pushed the sequence-folds branch 2 times, most recently from 209374c to 8865b01 Compare July 11, 2025 16:11
@treeowl treeowl force-pushed the sequence-folds branch 6 times, most recently from b89264c to 95b26ed Compare July 11, 2025 18:05
@treeowl treeowl marked this pull request as draft July 15, 2025 18:57
@treeowl
Copy link
Contributor Author

treeowl commented Jul 15, 2025

Hmm.... Benchmarks are not working out at all. I don't yet know what goes wrong.

@meooow25
Copy link
Contributor

@meooow25 Do you have any general thoughts?

I haven't seen all of it, but the implementations look alright.

Hmm.... Benchmarks are not working out at all. I don't yet know what goes wrong.

I'm not sure what you mean. For me tests and benchmarks seem to be running fine.
I looked at foldMap in particular:

  folds 10000
    foldMap_elem:        OK
      95.2 μs ± 5.3 μs, 184 B  allocated,   2 B  copied,  10 MB peak memory
    foldMap_traverseSum: OK
      119  μs ± 7.7 μs, 391 KB allocated,  28 B  copied,  10 MB peak memory
  folds 10000
    foldMap_elem:        OK
      75.1 μs ± 7.1 μs,  56 B  allocated,   2 B  copied,  10 MB peak memory
    foldMap_traverseSum: OK
      650  μs ±  28 μs, 1.9 MB allocated, 383 B  copied,  10 MB peak memory

foldMap_traverseSum is slower, but that seems to be mostly due to a missing {-# INLINABLE foldMap #-}. With that, I see

  folds 10000
    foldMap_elem:        OK
      73.6 μs ± 5.9 μs,  56 B  allocated,   2 B  copied,  10 MB peak memory
    foldMap_traverseSum: OK
      145  μs ± 6.6 μs, 391 KB allocated,  18 B  copied,  10 MB peak memory

This is not a problem today since it is just the coerced foldMap for FingerTree. In fact, why not improve Foldable FingerTree and leave Foldable Seq as it is?

Regarding the slightly bad 119 -> 145, I can check later to see what's happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants